Fold Typographic Punctuation in pr_review.py reply --match - #1878
Conversation
A --match string copied verbatim from a rendered finding fails to select the thread it was copied from when the finding's typographic quotes, dashes, or ellipsis don't survive the copy in ASCII, and NO_MATCH reads identically to the thread being missing or already resolved. matching_threads() now folds both the pattern and each candidate body through an ASCII table before the substring compare, and NO_MATCH now states how many unresolved threads exist so a zero and a several no longer read the same. A parameterized test checks each of the issue's seven characters independently of the fold table itself, so dropping one from the table still fails the test that covers it. Closes on promotion: #1299 Co-Authored-By: Claude Sonnet 5 <[email protected]>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Repository: ptr727/ProjectTemplate/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The functional change is small and well-covered by new tests, with only a minor test-robustness improvement suggested.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
This pull request improves scripts/pr_review.py reply --match so it can reliably select threads even when the thread text or the copied --match pattern differs only by typographic punctuation (curly quotes, en/em dashes, ellipsis), and it makes NO_MATCH refusals more actionable by including the total unresolved-thread count.
Changes:
- Add a typographic-to-ASCII folding table and apply it to both the
--matchneedle and candidate thread bodies before case-insensitive substring matching. - Extend the
NO_MATCHmessage to include the total number of unresolved threads on the PR. - Add targeted tests covering the fold behavior per character and the improved
NO_MATCHmessage.
| File | Description |
|---|---|
| scripts/pr_review.py | Folds typographic punctuation on both sides of --match comparisons; includes unresolved-thread count in NO_MATCH. |
| scripts/tests/test_pr_review.py | Adds regression tests for typographic folding and the updated NO_MATCH output. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
test_every_documented_character_selects_across_the_ascii_boundary captures stdout once in setUp and ran all seven fold checks against that same buffer, so a later iteration's assertIn found an earlier iteration's leftover REPLIED_AND_RESOLVED output rather than its own. Clearing the buffer at the start of each subTest makes the assertion specific to its own iteration. Co-Authored-By: Claude Sonnet 5 <[email protected]>
|
Re: the stdout-buffer finding on scripts/tests/test_pr_review.py:5266 (thread already shows resolved, so answering here for the record rather than via reply --resolve, which only targets unresolved threads): fixed in 7818762. Each subTest iteration in test_every_documented_character_selects_across_the_ascii_boundary now clears the shared stdout buffer (self.out.seek(0); self.out.truncate(0)) before wiring and running its own reply, so its assertIn checks only its own iteration's output rather than one accumulated across all seven. Swept the file for the same setUp-captures-stdout-once-then-loops-subTests shape in every other test class (TestCoverageExitCodes, TestCli, CommentCase, ReplyCase) and found no other instance of the same latent bug. |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The change is narrowly scoped, covered by new unit tests, and the updated matching behavior is applied symmetrically to both the pattern and candidate text.
Review effort: Lite
Findings: None
Resolved since last review (1)
…and Overnight Fixes to Main (#1850) Promotes `develop` to `main`. ## Carried - #1846: exits `repo-config/configure.sh` early, writing nothing, for a repository whose registry `status` is `archived`. - #1848: fails the validator's C# unit test step when the run wrote no non-empty Cobertura report, clearing `./coverage` first. It declines #1134's `ref` input with evidence, since a bare checkout already validates `github.sha`, and D1.2 now says so. #1134's third gap moved to #1800. - #1851: asserts that the two planted registration defects are themselves reported. - #1854, #1858: correct #1846's test docstrings, which claimed the script makes no `gh` call before the archived check in cases where it does. Raised by Copilot on this pull request. - #1856: passes `--repo` on documented handoff commands and guards handoff reads against a full page, per #1847. - #1859, #1863: fall back to the default search path when `PATH` is unset in #1848's and #1846's test harnesses. Raised by Copilot on this pull request. - #1861: scopes `VerifyReferenceAotCompatibility` to an AOT publish in `dotnet-codestyle`, per #1857. - #1867: establishes the `PATH` order `tool_shadow_path` names, per #1644. - #1870: makes the installer's dirty-checkout tests independent of the real checkout's state, per #1641. - #1873: pins and decodes git's quoting in `repo_gate.py`'s `ls-files` read, per #1580 and #1872. - #1878: folds typographic punctuation in `pr_review.py reply --match`, per #1299. - #1883: routes `configure.sh`'s `ruleset_id()` through `jqr`, per #1253. - #1885: states the pin comment as the release tag and defines `$/`, per #1805. - #1888: distinguishes `./` from `$/` resolution in the pin rule's prose, per #1886. - #1893: drops the issue reference from `repo-config/README.md`'s archived-exemption note, which Copilot flagged on six rounds of this pull request. - #1895: describes IL3058 in `dotnet-codestyle` as a referenced assembly lacking `IsAotCompatible` metadata set to `true`, and drops the unversioned package examples. Raised by CodeRabbit on this pull request. Callers that pin a hub release get the new C# check on their next pin bump. A test project that runs `dotnet test --coverage` without writing a report now fails its step rather than passing silently. Closes #1134 Closes #1847 Closes #1857 Closes #1644 Closes #1641 Closes #1580 Closes #1872 Closes #1299 Closes #1253 Closes #1805 Closes #1886 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Backlog counts and rankings now consistently exclude handoff issues, including those also marked blocked. * Repository configuration commands now exit without writing when a repository is archived. * Review-thread matching handles typographic punctuation, and no-match responses report the unresolved-thread count. * Tool setup handles PATH entries more precisely, and repository checks report unusual file paths without crashing. * **Reliability** * Validation now fails when C# or Python tests produce no coverage report. * Agent setup can use an explicit dirty-checkout override. * Workflow and repository guidance clarifies reference resolution, release-tag pinning, and AOT configuration. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Summary
A
--matchstring copied verbatim from a rendered finding fails to select the thread it was copied from when the finding's typographic quotes, dashes, or ellipsis don't survive the copy in ASCII, andNO_MATCHreads identically to the thread being missing or already resolved.matching_threads()now folds both the--matchpattern and each candidate thread body through an ASCII table (_TYPOGRAPHIC_FOLD) before the case-insensitive substring compare, covering the typographic single/double quotes, en/em dash, and ellipsis.NO_MATCHnow states how many unresolved threads exist on the pull request, so a zero and a several no longer read the same, per the issue's secondary suggestion.Closes on promotion: #1299
Two related gaps found during local review are filed separately as out of scope for this fix: #1876 (an ASCII
--and a real em dash still miss each other) and #1877 (a--matchpattern copied from this script's own truncated/whitespace-collapsedunresolved:line can still miss).Test plan
python3 -m unittest scripts.tests.test_pr_review(434 tests, all passing)uvx ruff@latest format --checkanduvx ruff@latest checkclean on both changed filesuvx pyrightshows only the same 2 pre-existing errors present ondevelopbefore this changescripts/local_review.py🤖 Generated with Claude Code